-
-
Notifications
You must be signed in to change notification settings - Fork 30.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-119180: Documentation for PEP 649 and 749 #122235
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't look at everything nor had I marked every typo but here's a first round of comments.
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left a few comments.
General points -- the line lengths in the new text seem a bit long. Also, I wonder if it is worth covering / providing call-outs to the interactions with the annotations
future? I imagine that will be something many readers will be thinking about.
A
``from __future__ import annotations`` was previously scheduled to | ||
become mandatory in Python 3.10, but the Python Steering Council | ||
twice decided to delay the change | ||
(`announcement for Python 3.10 <https://mail.python.org/archives/list/python-dev@python.org/message/CLVXXPQ2T2LQ5MP2Y53VVQFCXYWQJHKZ/>`__; | ||
`announcement for Python 3.11 <https://mail.python.org/archives/list/python-dev@python.org/message/VIZEBX5EYMSYIJNDBF6DMUMZOCWHARSO/>`__). | ||
No final decision has been made yet. See also :pep:`563` and :pep:`649`. | ||
become mandatory in Python 3.10, but the change was delayed and ultimately | ||
canceled. This feature will eventually be deprecated and removed. See | ||
:pep:`649` and :pep:`749`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we provide or link to migration guidance here, especially given that this is the first time a __future__
won't become the actual future?
Doc/library/annotationlib.rst
Outdated
The :mod:`!annotationlib` module provides tools for introspecting :term:`annotations <annotation>` | ||
on modules, classes, and functions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potentially this could be added later, but I think a longer narrative summary of annotationlib
and where it fits in could be of use -- we jump straight into functions here. Contrast with pathlib, heapq, hashlib.
I wonder if there are words from 749 that we could incorporate? Effectively, something that briefly talks to where this module fits into the landscape, perhaps aimed at the fairly experienced user of types
/inspect
/typing
, but someone who hasn't followed the PEPs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I was thinking of adding that too, but ran out of energy while working on the initial version. :) I'll probably work on this later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wrote such a summary now, along with a historical account of annotation semantics, since that seems like something the docs should cover.
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>
Doctest failures:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these are nits. I got curious about the PR and found some small things 🙈 !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks good @JelleZijlstra. I'm wondering if we are going a bit too deep into implementation in the Data Model Language Reference page.
Co-authored-by: Carol Willing <carolcode@willingconsulting.com>
Thanks for the review @willingc! Which part do you think goes into too much detail? I looked over the data model changes and all the changes seem relevant to me. |
@JelleZijlstra I reread the data model changes again on the rendered page (instead of the diff). On the initial read, I thought there was implementation content mixed in, but I was mistaken and misled by the rST markup. Thanks for rechecking and sorry for the extra noise. Let's 🚢 this. 😄 |
📚 Documentation preview 📚: https://cpython-previews--122235.org.readthedocs.build/